-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add google cloud configuration object for kubeclient #88
Conversation
46e6b8f
to
e1fb1ad
Compare
@@ -28,7 +29,7 @@ def build_v1beta1_kubeclient(context) | |||
end | |||
|
|||
def _build_kubeclient(api_version:, context:, endpoint_path: nil) | |||
config = Kubeclient::Config.read(ENV.fetch("KUBECONFIG")) | |||
config = GoogleConfig.read(ENV.fetch("KUBECONFIG")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sold about naming because someone reading this code could think that it only supports GKE config. What about KubeconfigWithGoogleSupport
or whatever?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont have any particular attachment to the name, though I'm not a huge fan of enormous names either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about GoogleConfig
incorrectly telling someone that the tool only supports google auth?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about just ConfigWithGoogle
? I'm just not a huge fan of having KubernetesDeploy::KubeclientBuilder::KubeconfigWithGoogleSupport
scopes = ['https://www.googleapis.com/auth/cloud-platform'] | ||
authorization = Google::Auth.get_application_default(scopes) | ||
|
||
authorization.apply({}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it need an empty apply? Can you add a comment to the code with the reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well apply
adds the access token to a set of headers in a hash. Since we just want to recover the token we don't need to pass in anything.
WebMock.allow_net_connect! | ||
end | ||
|
||
def test_auth_use_default_gcp_success |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it also makes sense to check that the config works as usual if auth provider isn't GCP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
class GoogleConfig < Kubeclient::Config | ||
def fetch_user_auth_options(user) | ||
if user['auth-provider'] && (user['auth-provider']['name'] == 'gcp') | ||
{ bearer_token: self.get_token } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is self
required here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not anymore.
559afa1
to
ec5fdbc
Compare
ec5fdbc
to
1237b20
Compare
I've addressed the review comments the configuration is now called |
end | ||
|
||
def set_google_env_vars | ||
ENV["GOOGLE_PRIVATE_KEY"] ||= "FAKE" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's probably a reason why it's necessary, but I'd prefer them to be reset on teardown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its because the googleauth
gem does a bunch of magic inspection to determine how to load configuration for a host (ie checking if creds are locally stored on disk, whether this is a GCE host, etc...).
Kubeclient doesn't support GCE authentication and there is currently no plan to merge the PR that would add support for it. To solve our problem I've added a small wrapper class based off the kubeclient PR. It provides a subclass of the
Kubeclient::Config
which will look for the approriate auth type and if found will fetch a new (1 hour lifetime) token.Kubeclient GCE PR
@jeremywadsack, since you were the author of the original PR you may be interested in this.